-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for dynamic arg sets #819
Conversation
This allows construction of basic_format_args from a dynamic set of arguments. The syntax is a little clunky and could probably be improved but this at least enables the functionality.
Related to issue #814 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. It looks good, just one minor comment.
test/format-test.cc
Outdated
args.emplace_back(fmt::internal::make_arg<ctx>("abc1")); | ||
args.emplace_back(fmt::internal::make_arg<ctx>(1.2f)); | ||
|
||
std::string result = fmt::vformat("{} and {} and {}", fmt::basic_format_args<ctx>(args.data(), (unsigned)args.size())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the cast is necessary here. Also please restring the line width to 80 chars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast is necessary; the size is being truncated. On MSVC for example:
warning C4267: 'argument': conversion from 'size_t' to 'fmt::v5::basic_format_args<fmt::v5::format_context>::size_type', possible loss of data
I can change it to a static_cast if that makes you happier.
@vitaut This patch doesn't work as expected.
result is In the above case it's not relevant (just a bit more typing as it's just a tag to call a different constructor...) but this causes issues with named arguments:
Patch introduces Asan triggers in format.h:1449 once it tries to dereference args.args_[2]. The fix for this case is to just add the terminator but it's confusing since args.size() was already provided:
|
The |
Just for completeness:
Returns And also as a side note I'd expect the output to be |
@MikePopoloski, could you please review the updated CONTIBUTING document, particularly the part about licensing, and let me know if you agree with it being applied to your contributions to {fmt}? The library is likely to be relicensed (#1073) so I'm collecting approval from all earlier contributors. Thanks! |
Just curious: I don't see any way to achieve this with the std::format (according to the C++ spec). Were there any discussions about standardizing this? |
There was no such discussion. |
Looking back at this thread, it seems like I never responded to @vitaut 's question above? If so I sincerely apologize, and if still relevant I agree with whatever licensing changes you want to make. Sorry! |
This allows construction of basic_format_args from a dynamic set of arguments. The syntax is a little clunky and could probably be improved but this at least enables the functionality.